fix abortsignal: make AbortSignal.timeout mock-timers compatible #60587

DeveloperViraj · 2025-11-05T11:56:00Z

AbortSignal.timeout() previously used internal/timers.setTimeout, which was not intercepted by mock.timers.enable().

This change updates it to use globalThis.setTimeout and globalThis.clearTimeout, so mock.timers.tick() correctly triggers aborts.

/cc @nodejs/testing

Notes for reviewers

This change replaces require('internal/timers') usage with global timers,
so mock.timers in the Node test runner can intercept AbortSignal.timeout.

Verified locally that the fix aligns with issue reproduction steps.

Renegade334

The DOM standard does not use setTimeout for this method, and changes to the public-facing timers API should not be observable here.

The correct route would probably be to add this as a discrete API target for the mocker (ie. enabled via mocks.timers.enable({ apis: ['AbortSignal.timeout'] })) with its own implementation.

ErickWendel · 2025-11-14T18:32:08Z

would you also add tests for this feat?

DeveloperViraj · 2025-11-15T13:41:51Z

Thanks a lot for the guidance!
I updated the mock implementation to use the separate API target approach (AbortSignal.timeout) as recommended, and added a new test for it.
Please let me know if I should improve or extend anything.

Renegade334

Thanks for working on this! There are a few changes to make, but this is definitely heading in the right direction.

Renegade334 · 2025-11-15T16:34:59Z

lib/internal/test_runner/mock/mock_timers.js

+  #storeOriginalAbortSignalTimeout() {
+    try {
+      this.#realAbortSignalTimeout = ObjectGetOwnPropertyDescriptor(AbortSignal, 'timeout');
+    } catch {
+      this.#realAbortSignalTimeout = undefined;
+    }
+  }


The store/restore methods should not be allowed to fail silently.

Suggested change

#storeOriginalAbortSignalTimeout() {

try {

this.#realAbortSignalTimeout = ObjectGetOwnPropertyDescriptor(AbortSignal, 'timeout');

} catch {

this.#realAbortSignalTimeout = undefined;

}

}

#storeOriginalAbortSignalTimeout() {

this.#realAbortSignalTimeout = ObjectGetOwnPropertyDescriptor(AbortSignal, 'timeout');

}

Renegade334 · 2025-11-15T16:35:33Z

lib/internal/test_runner/mock/mock_timers.js

+  #restoreOriginalAbortSignalTimeout() {
+    try {
+      if (this.#realAbortSignalTimeout) {
+        ObjectDefineProperty(AbortSignal, 'timeout', this.#realAbortSignalTimeout);
+      } else {
+        try {
+          delete AbortSignal.timeout;
+        } catch {}
+      }
+    } catch {}
+  }


Suggested change

#restoreOriginalAbortSignalTimeout() {

try {

if (this.#realAbortSignalTimeout) {

ObjectDefineProperty(AbortSignal, 'timeout', this.#realAbortSignalTimeout);

} else {

try {

delete AbortSignal.timeout;

} catch {}

}

} catch {}

}

#restoreOriginalAbortSignalTimeout() {

ObjectDefineProperty(AbortSignal, 'timeout', this.#realAbortSignalTimeout);

}

Renegade334 · 2025-11-15T16:35:57Z

 true

Please delete this artifact!

Renegade334 · 2025-11-15T16:38:34Z

test/addons/addons.status

I'm not sure how 0694adb made it into your branch. You will need to drop this commit, using interactive rebase for example.

Renegade334 · 2025-11-15T16:39:38Z

lib/internal/test_runner/mock/mock_timers.js

-  /**
-   * Advances the virtual time of MockTimers by the specified duration (in milliseconds).
-   * This method simulates the passage of time and triggers any scheduled timers that are due.
-   * @param {number} [time] - The amount of time (in milliseconds) to advance the virtual time.
-   * @throws {ERR_INVALID_STATE} If MockTimers are not enabled.
-   * @throws {ERR_INVALID_ARG_VALUE} If a negative time value is provided.
-   */


I'm not sure why these are being removed – unintentional AI action perhaps? Please restore these.

Renegade334 · 2025-11-15T16:39:52Z

lib/internal/test_runner/mock/mock_timers.js

-  /**
-   * @typedef {{apis: SUPPORTED_APIS;now: number | Date;}} EnableOptions Options to enable the timers
-   * @property {SUPPORTED_APIS} apis List of timers to enable, defaults to all
-   * @property {number | Date} now The epoch to which the timers should be set to, defaults to 0
-   */
-  /**
-   * Enables the MockTimers replacing the native timers with the fake ones.
-   * @param {EnableOptions} [options]
-   */


Renegade334 · 2025-11-15T16:39:58Z

lib/internal/test_runner/mock/mock_timers.js

    internalOptions.apis ||= SUPPORTED_APIS;

    validateStringArray(internalOptions.apis, 'options.apis');
-    // Check that the timers passed are supported


Renegade334 · 2025-11-15T16:40:24Z

lib/internal/abort_controller.js

-  clearTimeout,
-  setTimeout,
-} = require('timers');
+  clearTimeout: internalClearTimeout,
+  setTimeout: internalSetTimeout,
+} = require('internal/timers');
+
+const clearTimeout = globalThis.clearTimeout;
+const setTimeout = globalThis.setTimeout;
+


Please revert the original changes to lib/internal/abort_controller.

Renegade334 · 2025-11-15T16:51:06Z

lib/internal/test_runner/mock/mock_timers.js

+        'AbortSignal.timeout': () => {
+          this.#storeOriginalAbortSignalTimeout();
+
+          const mock = this;
+
+          ObjectDefineProperty(AbortSignal, 'timeout', {
+            __proto__: null,
+            configurable: true,
+            writable: true,
+            value: function mockableAbortSignalTimeout(delay) {
+              if (NumberIsNaN(delay)) {
+                throw new ERR_INVALID_ARG_VALUE('delay', delay, 'delay must be a number');
+              }
+
+              const controller = new AbortController();
+
+              const timer = mock.#setTimeout(
+                () => {
+                  try {
+                    controller.abort();
+                  } catch {}
+                },
+                delay
+              );
+
+              try {
+                ObjectDefineProperty(controller.signal, '__mockTimer', {
+                  __proto__: null,
+                  configurable: true,
+                  writable: true,
+                  value: timer,
+                });
+              } catch {}
+
+              return controller.signal;
+            },
+          });
+        },


Again, nothing here should be able to fail silently; please remove the try/catch blocks.

Renegade334 · 2025-11-15T16:57:44Z

lib/internal/test_runner/mock/mock_timers.js

+              try {
+                ObjectDefineProperty(controller.signal, '__mockTimer', {
+                  __proto__: null,
+                  configurable: true,
+                  writable: true,
+                  value: timer,
+                });
+              } catch {}


I don't think there's any need to define this; no underlying timer is exposed on true AbortSignals, and the timeout is protected from GC while it remains active in the execution queue.

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 5, 2025

Renegade334 reviewed Nov 6, 2025

View reviewed changes

Renegade334 requested changes Nov 15, 2025

View reviewed changes

Renegade334 added semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues and PRs related to the test runner subsystem. labels Nov 15, 2025

DeveloperViraj closed this Nov 15, 2025

DeveloperViraj force-pushed the fix/mock-timers-abortsignal-timeout branch from c30dce3 to 735ee27 Compare November 15, 2025 20:43

Uh oh!

fix abortsignal: make AbortSignal.timeout mock-timers compatible #60587

fix abortsignal: make AbortSignal.timeout mock-timers compatible #60587

Uh oh!

Conversation

DeveloperViraj commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes for reviewers

Uh oh!

Renegade334 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErickWendel commented Nov 14, 2025

Uh oh!

DeveloperViraj commented Nov 15, 2025

Uh oh!

Renegade334 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DeveloperViraj commented Nov 5, 2025 •

edited

Loading

Renegade334 left a comment •

edited

Loading